Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(perf): Cache commutative instructions in constant folding #5832

Closed
wants to merge 3 commits into from
Closed

feat(perf): Cache commutative instructions in constant folding #5832

wants to merge 3 commits into from

Conversation

vezenovm
Copy link
Contributor

Description

Problem*

Resolves

Summary*

Keeping a draft as I want to see how it runs in the code size benchmarks

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

jfecher
jfecher reviewed Aug 27, 2024
View reviewed changes
compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs Outdated
Comment on lines 289 to 301
if let Instruction::Binary(binary) = &instruction {
match binary.operator {
BinaryOp::Add
| BinaryOp::Mul
| BinaryOp::Eq
| BinaryOp::And
| BinaryOp::Or
| BinaryOp::Xor => {
let commutative_instruction = Instruction::Binary(Binary {
lhs: binary.rhs,
rhs: binary.lhs,
operator: binary.operator,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this hard check here, we should sort the operands when creating a commutative binary op. E.g. lower ValueIds are always the lhs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. However, we seem to be getting similar results as before.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 27, 2024 •
edited
Loading

Changes to Brillig bytecode sizes

Generated at commit: 6605711922cbb8ee068b8cdbbd9b00c869b47fcf, compared to commit: 3c778b73d9458ab708df21c850468d708676cde4

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
binary_operator_overloading -2 ✅ -0.44%
Full diff report 👇
Program Brillig opcodes (+/-) %
binary_operator_overloading 450 (-2) -0.44%

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 27, 2024 •
edited
Loading

Changes to circuit sizes

Generated at commit: 6605711922cbb8ee068b8cdbbd9b00c869b47fcf, compared to commit: 3c778b73d9458ab708df21c850468d708676cde4

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression_5252 +72 ❌ +0.09% +72 ❌ +0.07%
slices +1 ❌ +0.32% +1 ❌ +0.03%
Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression_5252 77,070 (+72) +0.09% 97,311 (+72) +0.07%
slices 312 (+1) +0.32% 3,381 (+1) +0.03%
hashmap 200,183 (+10) +0.00% 361,213 (+10) +0.00%
conditional_1 4,784 (-3) -0.06% 38,799 (0) 0.00%
merkle_insert 1,780 (+1) +0.06% 29,032 (0) 0.00%
slice_dynamic_index 1,201 (+1) +0.08% 6,420 (0) 0.00%

@jfecher
Copy link
Contributor

jfecher commented Aug 27, 2024

I don't understand the regression_5252 size increase

@vezenovm
Copy link
Contributor Author

I don't understand the regression_5252 size increase

Yeah I don't either. I guess we can close this PR as it doesn't seem to be providing many benefits.

jfecher
jfecher reviewed Aug 27, 2024
View reviewed changes
compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
Comment on lines +198 to +200
// Sort operands of commutative instructions
if let Instruction::Binary(binary) = &instruction {
match binary.operator {
Copy link
Contributor

@jfecher jfecher Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I meant sorting when the instructions are first created. Not during constant folding. Probably won't remove the size increase in the tests though.

@vezenovm
Copy link
Contributor Author

Going to close this PR as the benefits seem minimal in Brillig code size and actually cause regressions for ACIR.

@vezenovm vezenovm closed this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfecher jfecher jfecher left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vezenovm @jfecher